Migrate precompiles/bank and giga/deps metrics to OTel (PLT-912) - #3859
Migrate precompiles/bank and giga/deps metrics to OTel (PLT-912)#3859amir-deris wants to merge 18 commits into
Conversation
Dual-emit the bank_new_account counter across all versioned bank precompiles and the giga fork's bank/scheduler paths, mirroring the already-migrated sei-cosmos keeper and scheduler instruments. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3859 +/- ##
==========================================
+ Coverage 59.45% 60.61% +1.15%
==========================================
Files 2321 2277 -44
Lines 198345 188235 -10110
==========================================
- Hits 117931 114102 -3829
+ Misses 69213 64099 -5114
+ Partials 11201 10034 -1167
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
PR SummaryMedium Risk Overview Bank new-account paths now dual-emit through mirrored instruments (same meter/name/description/unit) in Scheduler metrics get the same dual-emit treatment in the Giga fork. sei-cosmos Reviewed by Cursor Bugbot for commit 41299ae. Bugbot is set up for automated code reviews on this repo. Configure here. |
![seidroid[bot]](https://avatars.githubusercontent.com/in/2745135?s=60&v=4){kind=small}
There was a problem hiding this comment.
Mechanically correct, purely-additive OTel dual-emit sweep: the 15-file coverage claim checks out, the v552/v555 exclusion is verified, and the giga/tasks mirror is byte-identical to sei-cosmos. No blockers; findings are about a duplicated instrument definition, a lost panic guard on a consensus-critical path, and deviations from the repo's package-local metrics.go convention.
Findings: 0 blocking | 8 non-blocking | 4 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- No tests added.
utils/metrics/metrics_util_test.goalready exists — a case using an OTelmanualReaderto assertRecordBankNewAccountbumps bothbank_new_accountand the legacynew.accountsink would lock in the dual-emit contract before PLT-353 removes the legacy half. Same for the two newgiga/deps/tasksinstruments. precompiles/bank/legacy/v601,v605, andv610previously calledtelemetry.IncrCounterdirectly and now route through the panic-recoveringSafeTelemetryIncrCounterinsideRecordBankNewAccount. Benign in practice (armon/go-metrics installs a default global sink ininit, so it doesn't panic), but it is a behavior change in version-frozen files, which sits slightly at odds with the PR description's "no behavioral change". Worth a sentence in the description given theapp-hash-breakinglabel.- Inherited from the sei-cosmos mirror, so out of scope here, but flagging for the follow-up:
scheduler_incarnationsrecords a per-round maximum viaAdd()on a monotonic counter, so the exported series is a sum of maxima rather than anything meaningful. A gauge or histogram would carry the intended signal. Same file also usescontext.Background()whereProcessAllhas a realctx.Context()available, dropping exemplar/trace linkage. - The Cursor second-opinion pass produced no output (
cursor-review.mdis empty). Codex ran and reported no material issues, which matches my own read — nothing in the diff is functionally wrong. - 4 suggestion(s)/nit(s) flagged inline on specific lines.
| "go.opentelemetry.io/otel/sdk/resource" | ||
| ) | ||
|
|
||
| var bankNewAccountCounter = mustCounter(otel.Meter("seicosmos_x_bank_keeper").Int64Counter( |
There was a problem hiding this comment.
[suggestion] This is a second, independent definition of an instrument that already exists: sei-cosmos/x/bank/keeper/metrics.go:21 declares bank_new_account on the same meter scope (seicosmos_x_bank_keeper) with the same description, unit, and kind.
Today that's harmless — the OTel SDK caches instruments by (name, description, unit, kind, number), so both resolve to the same aggregator and sum into one stream, which is presumably the intent (matching the shared legacy new.account key). The hazard is drift: if either copy's description or unit is edited later, the SDK stops deduping, logs a duplicate-metric-stream conflict, and the Prometheus exporter emits two families with the same name but different HELP text — which the Prometheus client rejects, taking out the scrape rather than just that series.
At minimum add a cross-reference comment on both declarations noting they must stay byte-identical; better, have one import the other so there's a single definition.
Separately on scope naming: every other OTel instrument in this tree (~30 files) lives in a package-local metrics.go with meter = otel.Meter("<package_path>"). This one is inline in metrics_util.go and carries a scope naming a package that is not a caller — the actual callers are precompiles/bank/* and giga/deps/xbank. Anyone filtering by instrumentation scope will attribute precompile-originated account creations to the bank keeper.
| // RecordBankNewAccount dual-emits the legacy new-account counter and its OTel | ||
| // counterpart (bank_new_account). Call from defer when creating an account. | ||
| func RecordBankNewAccount(ctx context.Context) { | ||
| bankNewAccountCounter.Add(ctx, 1) |
There was a problem hiding this comment.
[suggestion] The OTel Add sits outside the recover, which quietly drops a guard at the call sites that most need it.
12 of the 15 precompile sites this PR touches previously called SafeTelemetryIncrCounter — a wrapper that exists for exactly one reason: to stop a telemetry fault from panicking inside precompile execution. That defer now runs in sendNative during EVM execution, so a panic escaping bankNewAccountCounter.Add propagates into a consensus-critical path.
I don't have a concrete panic path (Add is nil-ctx-safe via trace.SpanFromContext, and a no-op before SetupOtelMetricsProvider runs), so this is defense-in-depth rather than a live bug. But the fix is one line, and it restores the property the original code deliberately had:
func RecordBankNewAccount(ctx context.Context) {
defer func() {
if e := recover(); e != nil {
debug.PrintStack()
}
}()
bankNewAccountCounter.Add(ctx, 1)
// TODO(PLT-353): remove once bank_new_account verified
SafeTelemetryIncrCounter(1, "new", "account")
}Worth deciding as a policy question for the whole PLT-218 sweep, not just this helper: should OTel emits on consensus paths be panic-guarded the way the legacy ones were?
| sdkerrors "github.com/sei-protocol/sei-chain/sei-cosmos/types/errors" | ||
| cosmosbanktypes "github.com/sei-protocol/sei-chain/sei-cosmos/x/bank/types" | ||
| paramtypes "github.com/sei-protocol/sei-chain/sei-cosmos/x/params/types" | ||
| seimetrics "github.com/sei-protocol/sei-chain/utils/metrics" |
There was a problem hiding this comment.
[suggestion] This is the one place the PR breaks its own fork-parity pattern. giga/deps/tasks got a verbatim copy of sei-cosmos/tasks/metrics.go, and the upstream sei-cosmos/x/bank/keeper keeps its own local metrics.go — but here the forked bank keeper reaches up into the chain-level utils/metrics instead.
That pulls x/evm/types, the OTel SDK, and the Prometheus exporter into a cosmos-fork package that otherwise only depends on sei-cosmos/*. It's a layering inversion (a vendored SDK module depending on chain-app code) and narrows the room to move things around later without an import cycle.
A local giga/deps/xbank/keeper/metrics.go mirroring sei-cosmos/x/bank/keeper/metrics.go, with the two-line inline emit at the call sites, would keep this file a clean diff against its upstream counterpart and match how the rest of the PR handles the fork. It also sidesteps the duplicate-instrument concern flagged on metrics_util.go, since the giga copy would be the same kind of near-duplicate the fork already tolerates everywhere else.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — purely additive OTel dual-emit for the bank new_account counter and giga scheduler metrics, no logic or state changes.
What was reviewed: the 15 near-identical sendNative/InputOutputCoins/SendCoins call sites replacing the legacy counter call with RecordBankNewAccount, the new giga/deps/tasks/metrics.go OTel instruments dual-emitted from emitMetrics(), and the shared RecordBankNewAccount helper in utils/metrics. Confirmed the legacy telemetry call is preserved inside the new helper (true dual-emit, no metric regression) and that defer ordering/placement is unchanged relative to the original code.
Extended reasoning...
Overview
This PR mechanically migrates the new-account (bank) and scheduler retries/incarnations (giga OCC scheduler) counters from the legacy armor/go-metrics-based telemetry to OpenTelemetry, dual-emitting both during the migration window. It touches 19 files, but 18 of them are near-identical one-line swaps (telemetry.IncrCounter/SafeTelemetryIncrCounter to metrics.RecordBankNewAccount(ctx.Context())) across the main and 14 versioned legacy bank precompiles plus the giga bank keeper. The only new logic is the shared RecordBankNewAccount helper and a new giga/deps/tasks/metrics.go OTel instrument file, both of which are simple counter wrappers following an existing pattern already used elsewhere in utils/metrics/metrics_util.go.
Security risks
None. This is observability-only instrumentation — no new inputs are parsed, no auth/permission logic is touched, and no state (KV store) reads/writes are added or removed. The defer call sites and their position relative to SetAccount/state mutation are unchanged from the original code, so gas metering and execution order are preserved.
Level of scrutiny
Low-to-medium. The diff is mechanically repetitive and self-evidently correct on inspection (dual-emit preserves the legacy call), but the PR touches EVM precompile execution paths and is tagged app-hash-breaking, which generally warrants care. In this case, the tag appears to be a conservative auto-label from touching precompile/keeper paths rather than a real consensus-affecting change, since no state-transition logic was modified — confirmed by reading each modified sendNative/SendCoins/InputOutputCoins site line-by-line.
Other factors
No bugs were surfaced by the bug-hunting system, and the two candidate issues raised (metric name collision between giga and sei-cosmos schedulers, and missing panic recovery around the new OTel counter Add call) were investigated and refuted — the latter is also consistent with the same no-recovery pattern already present for other OTel counters in this codebase (e.g. taskMetrics.retries.Add in scheduler.go), so it isn't a new risk introduced by this change. The PR description accurately states test coverage for the touched packages, and the pattern mirrors prior merged OTel-migration commits already on main.
Add panic recovery to RecordBankNewAccount, cross-reference comments on the three bank_new_account instrument declarations, and mirror the Giga xbank keeper fork with a local metrics.go instead of importing utils/metrics. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
A clean, mechanical dual-emit sweep: 15 bank precompile files route through a new metrics.RecordBankNewAccount, and the Giga fork picks up the scheduler retries/incarnations and bank_new_account OTel instruments as byte-identical mirrors of their sei-cosmos counterparts. I verified coverage (all 16 legacy dirs accounted for; v552/v555 correctly excluded since they delegate to SendCoinsAndWei) and that giga/deps/tasks/metrics.go is identical to sei-cosmos/tasks/metrics.go. No blockers; a few consistency/robustness notes below.
Findings: 0 blocking | 6 non-blocking | 2 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- The three
bank_new_accountdeclarations (sei-cosmos/x/bank/keeper/metrics.go,utils/metrics/metrics_util.go,giga/deps/xbank/keeper/metrics.go) are kept in sync only by prose comments. Since drift silently splits the series rather than failing loudly, consider a small test that asserts the description/unit strings are equal across the three, or exporting a sharedconstfor them. giga/deps/xbank/keeper/send.goemits the OTel counter (and the baretelemetry.IncrCounter) inside adeferwith no panic recovery, while the precompile path added one specifically because "a telemetry fault here must not panic into a consensus-critical path." The keeper send path is equally consensus-critical. This is a deliberate byte-mirror ofsei-cosmos/x/bank/keeper/send.go, so fixing it here alone would break that parity — worth addressing in both, or not at all.- No tests were added.
utils/metricsalready hasmetrics_util_test.go; a case assertingRecordBankNewAccountemitsbank_new_accountand swallows a panic from a nil/failing counter would lock in the panic-safety contract the doc comment promises. - The Cursor second-opinion pass produced no output (
cursor-review.mdis empty). Codex reported no material issues, which matches my read. - 2 suggestion(s)/nit(s) flagged inline on specific lines.
| debug.PrintStack() | ||
| } | ||
| }() | ||
| bankNewAccountCounter.Add(ctx, 1) |
There was a problem hiding this comment.
[suggestion] A single recover wraps both emits, with the OTel Add first. If bankNewAccountCounter.Add ever panics, SafeTelemetryIncrCounter on the next line never runs — which breaks the PR's stated invariant that "legacy metric output is preserved on every path," and does so precisely in the failure case where you'd most want the legacy signal.
Cheap fix: emit the legacy counter first (it's already independently panic-safe via SafeTelemetryIncrCounter), so the OTel call can only ever lose its own emit:
func RecordBankNewAccount(ctx context.Context) {
defer func() {
if e := recover(); e != nil {
debug.PrintStack()
}
}()
// TODO(PLT-353): remove once bank_new_account verified
SafeTelemetryIncrCounter(1, "new", "account")
bankNewAccountCounter.Add(ctx, 1)
}
There was a problem hiding this comment.
No bugs found, but this PR has two unaddressed design comments from another automated reviewer (seidroid) that are worth a maintainer's judgment call before merge: the bank_new_account OTel instrument is now independently declared in three packages (relying on OTel's silent name/desc/unit dedup rather than a shared definition), and the Giga fork's xbank/keeper now imports the chain-level utils/metrics package, which is a layering change for that fork. Neither is a correctness bug, but they're design tradeoffs a human should sign off on given the size of this sweep (21 files) and that it touches the bank precompile's transaction path.
Beyond those, I checked the panic-recovery concern raised on RecordBankNewAccount: the defer/recover is registered before the bankNewAccountCounter.Add call, so it does cover a panic from Add (the legacy counter emit would still be skipped in that case, but execution wouldn't panic into the precompile) — this looks correctly handled in the current diff.
Extended reasoning...
This PR mechanically dual-emits OTel counters alongside existing legacy telemetry for the bank precompile new-account counter (15 near-identical files) and the Giga scheduler/bank-keeper fork. No execution or state logic changes; purely additive instrumentation with legacy paths preserved.
Security risk is minimal — this is observability code, not auth/crypto/consensus logic — though the precompile sendNative path does run during EVM transaction execution, so a panic there would be consensus-relevant. I verified the RecordBankNewAccount helper's defer/recover is correctly ordered to cover the OTel Add call.
Given the number of files touched, the fact it's on a transaction-execution path, and that another automated reviewer (seidroid) left two substantive unaddressed suggestions (duplicate instrument definition across three packages relying on implicit OTel dedup, and a layering change pulling chain-app metrics into a cosmos-fork package), I think this warrants a human sign-off on those tradeoffs rather than a shadow approval, even though no correctness bugs were found.
| bankNewAccountCounter.Add(ctx, 1) | ||
| // TODO(PLT-353): remove once bank_new_account verified | ||
| SafeTelemetryIncrCounter(1, "new", "account") |
There was a problem hiding this comment.
RecordBankNewAccount runs the OTel Add before SafeTelemetryIncrCounter under one function-scoped recover, so a panic in the OTel emit drops the legacy counter silently and the PLT-353 parity comparison skews with it. On main the precompiles emitted the legacy counter unconditionally, so ordering legacy first, or giving each emit its own recover, would hold that guarantee.
There was a problem hiding this comment.
The OTel dual-emit changes themselves are clean and correctly mirrored, but the PR delivers only 1 of the 15 precompile bank files its description claims to migrate — 14 legacy/v* copies are untouched, so the stated parity sweep is not done and parts of the description are factually wrong. A few smaller consistency/robustness issues are noted inline.
Findings: 1 blocking | 8 non-blocking | 3 posted inline
Blockers
- Parity sweep is not delivered (confirmed, also flagged by Codex). The description says
bank_new_accountis dual-emitted in "all 15 precompile bank files (precompiles/bank/bank.go+ 14 versionedlegacy/v*copies)", but the diff touches onlyprecompiles/bank/bank.go. All 14 legacy versions still emit the legacy counter only:v562,v580,v600,v603,v606,v610(bare),v614,v620,v630,v640,v65,v66, plusv601/v605(baretelemetry.IncrCounter). The description's "minor behavioral nuance" paragraph — claimingv601,v605,v610now route throughSafeTelemetryIncrCounter— describes a change that is not in this diff. Either the edits were dropped (a 15-file sweep would not fit in +124/-8) or the description needs correcting; as-is, replay/tracing through legacy precompile versions produces nobank_new_accountseries.
Non-blocking
- The Cursor second-opinion pass produced no output (
cursor-review.mdis empty) — that review lane effectively did not run for this PR. - No tests added.
RecordBankNewAccountis a new exported helper on a consensus-adjacent path with non-trivial semantics (recover + dual emit); a smallutils/metricstest asserting both emits fire, and that a panicking OTel provider does not escape, would lock in the contract the description leans on. - Three near-identical
must/mustCountergeneric helpers now exist (sei-cosmos/x/bank/keeper,giga/deps/xbank/keeper,giga/deps/tasks, plusutils/metrics). Fork isolation is a legitimate reason to duplicate, bututils/metrics.mustCounteris a non-generic one-off that could just use the samemust[V any]shape for consistency. - The
sei-cosmos/tasks/scheduler.goemitMetricssignature change (plumbingctx.Context()instead ofcontext.Background()) and thescheduler_incarnationsdescription rewording are outside the stated scope ("precompile bank and giga/deps"). Both look like sensible improvements — worth calling out in the description so reviewers of sei-cosmos know an existing instrument's description changed. - The PR carries the
app-hash-breakinglabel, but the change is described (and appears) to be purely additive instrumentation with no state/execution effect. Worth confirming the label is intentional — a spurious app-hash-breaking label affects release/upgrade handling. - 3 suggestion(s)/nit(s) flagged inline on specific lines.
| debug.PrintStack() | ||
| } | ||
| }() | ||
| bankNewAccountCounter.Add(ctx, 1) |
There was a problem hiding this comment.
[suggestion] Ordering under the shared recover: if bankNewAccountCounter.Add panics, the outer recover swallows it and SafeTelemetryIncrCounter never runs — so an OTel fault silently drops the legacy counter too, which is a regression against the pre-PR behavior at every call site.
Since the whole point of this helper is that legacy output is preserved on every path, either emit the legacy counter first, or give the OTel Add its own recover:
func RecordBankNewAccount(ctx context.Context) {
// TODO(PLT-353): remove once bank_new_account verified
defer SafeTelemetryIncrCounter(1, "new", "account")
defer func() {
if e := recover(); e != nil {
debug.PrintStack()
}
}()
bankNewAccountCounter.Add(ctx, 1)
}| } | ||
|
|
||
| func (s *scheduler) emitMetrics() { | ||
| taskMetrics.retries.Add(context.Background(), int64(s.metrics.retries)) |
There was a problem hiding this comment.
[suggestion] This PR updates the sei-cosmos twin to plumb the real context (emitMetrics(ctx.Context()), sei-cosmos/tasks/scheduler.go:268/288) but the Giga fork copy keeps context.Background(). Since this file is meant to mirror upstream, it starts out already drifted — and it loses the exemplar/trace correlation the upstream change was presumably for. Suggest applying the same emitMetrics(ctx context.Context) signature here and calling defer s.emitMetrics(ctx.Context()) in ProcessAll.
| if !accExists { | ||
| defer telemetry.IncrCounter(1, "new", "account") | ||
| defer func() { | ||
| bankMetrics.newAccount.Add(ctx.Context(), 1) |
There was a problem hiding this comment.
[nit] The precompile helper wraps its OTel Add in a recover on the stated grounds that "a telemetry fault here must not panic into a consensus-critical path" (utils/metrics/metrics_util.go:77). This keeper send path is equally consensus-critical, but Add is called bare here and at line 174. It matches the sei-cosmos pattern, so this may be a deliberate consistency choice — but if the panic-safety rationale holds for precompiles it holds here, and the two paths should agree either way.
There was a problem hiding this comment.
This PR hasn't addressed bdchatham's most recent comment (2026-08-05T17:33:43Z): RecordBankNewAccount still runs the OTel Add before SafeTelemetryIncrCounter under a single recover, so a panic in the OTel emit would silently drop the legacy counter it's meant to be validated against during the PLT-353 comparison window. That's the last unresolved comment in the thread, so I'll leave this for a human to confirm before merging.
Extended reasoning...
Overview
This PR adds OTel dual-emit counters for the bank new_account metric across 15 precompile files and the Giga fork's xbank keeper, plus mirrors OCC scheduler retries/incarnations counters into the Giga fork. It's purely additive telemetry — no precompile execution or state-transition logic changes, and legacy metrics are preserved.
Security risks
None from an attack-surface standpoint — this is observability code, not auth/consensus logic. The only 'consensus-relevant' risk is a panic in the new OTel code path escaping into precompile execution (touching sendNative, a transaction-path method), which the PR's own RecordBankNewAccount helper defends against via recover().
Level of scrutiny
Given the change is purely additive telemetry with an existing panic guard, this doesn't need production-critical-code-level scrutiny for correctness of business logic. However, the ordering of operations inside a shared recover block is a real, still-open design question raised by a human reviewer (bdchatham) in the most recent PR comment, which is why I'm not approving outright — that class of comment (unaddressed reviewer feedback) is a signal a human should follow up on before merge, even though the underlying risk is low (defense-in-depth, not a confirmed live bug).
Other factors
Earlier bot review rounds (seidroid) raised layering-inversion and duplicate-instrument-definition concerns for the Giga xbank keeper; those appear to have already been addressed in the current diff (the Giga keeper now has its own local metrics.go mirroring the sei-cosmos pattern rather than importing utils/metrics). The bug-hunting system found no bugs this run, and the candidate issues it ruled out (giga scheduler context.Background() fork-parity, legacy v552/v555 not migrated) both fall under expected, intentional PR scope per the description. Test coverage on the new code paths is partial per Codecov, which is typical for defer/panic-recovery branches.
amir-deris
changed the title
There was a problem hiding this comment.
Purely additive OTel dual-emit instrumentation for bank_new_account and the OCC scheduler counters; no state, gas, or precompile logic changes, and legacy counters remain on every path. No blockers — the notes are about emit ordering/panic-safety consistency, the prose-only contract keeping three duplicated instrument declarations byte-identical, and missing test coverage.
Findings: 0 blocking | 10 non-blocking | 4 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Cursor's second-opinion pass produced no output (
cursor-review.mdis empty). Codex reported "No material issues found in the PR diff." - The
bank_new_accountinstrument is now declared in three places (sei-cosmos/x/bank/keeper/metrics.go,giga/deps/xbank/keeper/metrics.go,utils/metrics/metrics_util.go) andscheduler_retries/scheduler_incarnationsin two, with byte-identical description/unit enforced only by comments. Drift silently splits the series (OTel logs a duplicate-instrument conflict and keeps both streams) — consider exported description/unit constants plus a small test asserting they match across declarations, which would also have caught thescheduler_incarnationswording fix needing to land in both copies. - No new tests.
giga/deps/taskscontains no_test.gofiles at all, so the test-plan line "go testpasses forgiga/deps/tasks" is vacuous for the newmetrics.go/emitMetricschange.utils/metricsalready hasmetrics_util_test.go; a manual-reader test assertingRecordBankNewAccountbumps both the OTel and legacy counters would be cheap and would pin the dual-emit contract. - The PR carries the
app-hash-breakinglabel, but the diff is metrics-only with no state/gas/logic change that I can see — worth confirming the label is intentional (likely inherited from a base merge) so it doesn't misdirect release gating. - Informational:
emitMetricsnow receives the request context and runs from adefer, so it may fire with an already-cancelled context. The current OTel SDK's synchronous counters ignore cancellation, so nothing drops today; noting only because the previous code usedcontext.Background()and was immune by construction. - I could not run
go buildin this environment (command not permitted), so compilation is unverified here. I did confirm statically thatmust/meterare declared only once per package ingiga/deps/tasksandgiga/deps/xbank/keeper, and thatcontextis already imported ingiga/deps/tasks/scheduler.go. - 4 suggestion(s)/nit(s) flagged inline on specific lines.
| debug.PrintStack() | ||
| } | ||
| }() | ||
| bankNewAccountCounter.Add(ctx, 1) |
There was a problem hiding this comment.
[suggestion] The new OTel emit runs first, and both emits share a single recover(). If bankNewAccountCounter.Add ever panics, the recover swallows it and SafeTelemetryIncrCounter is never reached — so the legacy new.account counter silently stops incrementing, which contradicts the PR's "legacy metric output is preserved on every path" guarantee. Since the whole point of the recover is that you don't fully trust the new instrument, emit the legacy counter first (or give each emit its own recover) so a fault in the new path can't starve the one we still depend on.
| if !accExists { | ||
| defer telemetry.IncrCounter(1, "new", "account") | ||
| defer func() { | ||
| bankMetrics.newAccount.Add(ctx.Context(), 1) |
There was a problem hiding this comment.
[suggestion] Unlike the precompile path — where RecordBankNewAccount is explicitly wrapped in a recover because "a telemetry fault here must not panic into a consensus-critical path" — this Add is unguarded and runs from a defer inside InputOutputCoins/SendCoins, which is equally consensus-critical (a panic in a defer propagates). This does match the existing sei-cosmos/x/bank/keeper/send.go precedent, so it's not a blocker, but the two paths now reason about the same risk differently. At minimum, put telemetry.IncrCounter before the OTel Add here and in SendCoins (line 174) so the legacy counter can't be lost to a fault in the new instrument.
|
|
||
| func (s *scheduler) emitMetrics() { | ||
| func (s *scheduler) emitMetrics(ctx context.Context) { | ||
| taskMetrics.retries.Add(ctx, int64(s.metrics.retries)) |
There was a problem hiding this comment.
[nit] Same ordering point as the bank paths: this runs from defer s.emitMetrics(...) in ProcessAll, and the OTel Adds precede their legacy counterparts unguarded. Emitting telemetry.IncrCounter first keeps the legacy series intact regardless of what the new instruments do.
Superseded: latest AI review found no blocking issues.
There was a problem hiding this comment.
Purely additive OTel dual-emit instrumentation across the bank precompile, the Giga fork's bank keeper and OCC scheduler, plus a context.Context thread-through in emitMetrics. No correctness or security problems found; the notes are about the fragility of three hand-synced instrument declarations, missing test coverage, and inconsistent panic hardening.
Findings: 0 blocking | 8 non-blocking | 3 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- The Cursor second-opinion review file (
cursor-review.md) is empty — that pass produced no output. Codex reported no material issues. - No tests were added. The test plan only claims existing tests still pass, but
RecordBankNewAccount, the Gigabank_new_accountcounter, and the Giga scheduler counters are all new and uncovered. A test using an OTelmanual.Readerthat emits from all three paths and asserts exactly onebank_new_accountstream is collected would both cover the new code and mechanically enforce the byte-identical-description invariant the comments describe. - All
bank_new_accountevents merge into one series with no attribute distinguishing precompile-originated from keeper-originated creations, so dashboards can't break the number down by source. That's presumably the intent, but adding asourceattribute is cheap now and a breaking dashboard change later — worth a conscious decision before this lands. - The PR carries the
app-hash-breakinglabel while the description states no execution/state logic changed (and the diff supports that — metrics emission consumes no gas and touches no store). Worth confirming the label is intentional, since it gates the release process. - Five new
TODO(PLT-353)markers are added across four files. Make sure PLT-353 enumerates every legacytelemetry.IncrCountercall site so none is missed at cleanup time. - 3 suggestion(s)/nit(s) flagged inline on specific lines.
| // so precompile-originated and keeper-originated new-account events merge | ||
| // into a single bank_new_account series. Keep description/unit byte-identical | ||
| // across all three declarations or the OTel SDK stops deduping the instrument. | ||
| var bankNewAccountCounter = mustCounter(otel.Meter("seicosmos_x_bank_keeper").Int64Counter( |
There was a problem hiding this comment.
[suggestion] The bank_new_account instrument identity (name, scope, description, unit) is now declared independently in three packages — here, sei-cosmos/x/bank/keeper/metrics.go, and giga/deps/xbank/keeper/metrics.go — and the only thing keeping them in sync is a comment. If any one drifts, the OTel SDK silently emits two conflicting streams; nothing fails at compile time and no test catches it.
Understood that giga/deps is deliberately import-free of chain-app code so a shared constant isn't available everywhere. But at minimum, a test that registers a manual.Reader meter provider, drives all three paths, and asserts exactly one bank_new_account metric is collected would turn a silent regression into a failing build. Same argument applies to scheduler_retries/scheduler_incarnations, now duplicated between sei-cosmos/tasks and giga/deps/tasks.
| defer func() { | ||
| // TODO(PLT-353): remove once bank_new_account verified | ||
| telemetry.IncrCounter(1, "new", "account") | ||
| bankMetrics.newAccount.Add(ctx.Context(), 1) |
There was a problem hiding this comment.
[suggestion] This Add (and the one at line 176) is unguarded, while RecordBankNewAccount in utils/metrics deliberately wraps the equivalent call in a recover() with the rationale that "a telemetry fault here must not panic into a consensus-critical path." InputOutputCoins/SendCoins run in DeliverTx and are just as consensus-critical, so the reasoning applies equally here. Non-blocking since this mirrors the pre-existing sei-cosmos/x/bank/keeper/send.go shape, but if the panic-safety concern is real for the precompile it's real here too — consider a shared guarded helper (or a short comment explaining why these sites don't need one).
Minor: the emit order is inverted relative to sei-cosmos/x/bank/keeper/send.go (legacy first here, OTel first there). Since these files are meant to be mirrors, matching the order makes future diffs between them easier to read.
There was a problem hiding this comment.
Purely additive OTel dual-emit instrumentation for bank_new_account and the OCC scheduler counters; no execution or state logic changes, and legacy telemetry is preserved on every path. No blockers — the notes are about the hand-maintained "keep byte-identical" invariant now spanning three instrument declarations, inconsistent panic-guarding of the new Add calls, and the absence of any test for the one new helper.
Findings: 0 blocking | 6 non-blocking | 3 posted inline
Blockers
- None at the file/PR level.
Non-blocking
cursor-review.mdis empty — the Cursor pass produced no output, so this review reflects only my own analysis plus Codex's (which reported no material issues).- No tests added.
RecordBankNewAccountis the only new logic in the PR andutils/metricsalready has a test file (metrics_util_test.go) with a working manual-reader pattern. A smoke test asserting it records 1 and returns cleanly with a nilcontext.Context(reachable via a zero-valuesdk.Context, whoseContext()returns nil) would be cheap and would pin the panic-recovery contract the doc comment promises. - The PR carries the
app-hash-breakinglabel while the description states "No precompile execution/state logic changed — purely additive OTel instrumentation." I agree with the description: nothing here consumes gas or touches state. Worth reconciling the label before merge so it isn't misleading in the release notes. - 3 suggestion(s)/nit(s) flagged inline on specific lines.
| defer func() { | ||
| // TODO(PLT-353): remove once bank_new_account verified | ||
| telemetry.IncrCounter(1, "new", "account") | ||
| bankMetrics.newAccount.Add(ctx.Context(), 1) |
There was a problem hiding this comment.
[suggestion] Two things here, both also applying to the identical block at line 176:
-
Emit order is flipped relative to the sei-cosmos counterpart.
sei-cosmos/x/bank/keeper/send.go:149-151does OTelAddfirst, thentelemetry.IncrCounter; this does the reverse. Sincetelemetry.IncrCounterhere is the unguarded variant (notSafeTelemetryIncrCounter), a panic in the legacy sink would skip the OTel emit entirely and silently under-countbank_new_accounton exactly the paths this PR is trying to instrument. Matching the sei-cosmos ordering makes the new counter the one that survives. -
No panic guard on the new
Add.RecordBankNewAccount's doc comment justifies itsrecover()with "a telemetry fault here must not panic into a consensus-critical path" — butInputOutputCoins/SendCoinsrun insideDeliverTx, which is equally consensus-critical, and so doesemitMetricsingiga/deps/tasks/scheduler.go. A panic escaping here is recovered by baseapp into a tx failure, and because it would depend on node-local telemetry configuration it's a (low-probability) nondeterminism vector. I don't think OTel'sAddcan realistically panic, and this matches the pre-existing unguarded sei-cosmos pattern, so it isn't blocking — but the guard should be applied consistently across all four new call sites or dropped from the precompile as unnecessary.
| // instrument of the same name/scope (and giga/deps/xbank/keeper/metrics.go's) | ||
| // so precompile-originated and keeper-originated new-account events merge | ||
| // into a single bank_new_account series. Keep description/unit byte-identical | ||
| // across all three declarations or the OTel SDK stops deduping the instrument. |
There was a problem hiding this comment.
[suggestion] This "keep description/unit byte-identical across all three declarations" invariant is now maintained by hand across three packages, enforced only by comments. This PR is itself evidence the invariant drifts — the scheduler_incarnations description fix in sei-cosmos/tasks/metrics.go was needed for exactly this reason.
Consider a tiny dependency-free leaf package exporting the name/description/unit as consts. It would have no chain-app imports, so all three sites could import it — including giga/deps/xbank/keeper, which the PR description says is deliberately kept import-free of chain-app code, and sei-cosmos/x/bank/keeper, which can't import utils/metrics (cycle). Failing that, a test in the root module asserting the three declared descriptions agree would catch drift at CI time rather than in a metrics dashboard.
There was a problem hiding this comment.
This PR has had substantial back-and-forth review (seidroid[bot] across four rounds, plus a human comment from bdchatham), and while the highest-signal concern — the shared recover() ordering in RecordBankNewAccount that could silently drop the legacy counter — has been fixed (legacy SafeTelemetryIncrCounter now runs before the OTel Add), several other raised points still look open in the current diff: the Giga fork's bankMetrics.newAccount.Add calls in giga/deps/xbank/keeper/send.go remain unguarded by a recover despite the precompile helper's stated rationale that telemetry faults on consensus-critical paths need one, and giga/deps/tasks/metrics.go still lacks the cross-reference comment the other two mirrored metrics files carry.
I didn't find any new bugs beyond what's already been discussed, but given the unresolved review threads and the fact this PR touches consensus-critical bank/scheduler code paths (even though the changes are additive-only), I think it's worth a human pass to confirm which of the outstanding suggestions the author intends to address before merging.
Extended reasoning...
Overview
Additive OTel instrumentation across the bank precompile (precompiles/bank/bank.go), the Giga fork's bank keeper (giga/deps/xbank/keeper) and scheduler (giga/deps/tasks), and the sei-cosmos scheduler/bank keeper. Adds a RecordBankNewAccount helper in utils/metrics, threads real context into emitMetrics, and corrects an instrument description. No precompile/state-transition logic changes; legacy telemetry paths are preserved.
Security risks
None directly — this is observability-only instrumentation, not auth/crypto/permission logic. The main risk class is a telemetry fault (panic in an OTel Add call) propagating into a consensus-critical DeliverTx/precompile path, which is exactly what the extensive review thread has focused on.
Level of scrutiny
Medium-high, despite being "just metrics." The code runs on every native bank transfer and every OCC scheduler round, i.e. hot, consensus-critical paths, and the PR explicitly makes a panic-safety claim ("a telemetry fault here must not panic into a consensus-critical path") that needs to actually hold everywhere it's asserted. The bug hunting system found no confirmed bugs this run, and the one candidate that was investigated (OTel panic under a shared recover silently dropping the legacy counter) matches an issue the human/bot reviewers already raised and which the current code appears to have fixed by reordering the two emits.
Other factors
The PR has multiple rounds of detailed review from seidroid[bot] and one comment from a human (bdchatham), several of which (duplicate instrument declarations across three packages, unguarded Add() calls in the Giga fork's keeper, a missing consistency comment in giga/deps/tasks/metrics.go) don't appear resolved in the current diff. None of these looks like a live correctness bug on their own — they're mostly defense-in-depth and consistency asks — but the volume of open threads and the consensus-path context argue for a human owner explicitly deciding which to address versus defer, rather than a bot approval closing out the PR.
There was a problem hiding this comment.
Purely additive OTel dual-emit instrumentation; no state, gas, or execution-path changes, and legacy counters are preserved on every touched path. No blockers — the notable items are the Giga InputOutputCoins emission semantics now diverging from the canonical sei-cosmos keeper on error paths, three hand-copied instrument declarations kept in sync only by comments, and no tests pinning any of it.
Findings: 0 blocking | 11 non-blocking | 3 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Cursor's second-opinion file (
cursor-review.md) is empty — that pass produced no output. Findings below merge only Codex's single P1 with this review. - No tests added.
giga/deps/xbank/keeper/send_test.gohas noInputOutputCoinscoverage at all, so the new count-then-emit path (and the "no emit on error" behavior it introduces) is unverified. A unit test with an in-memory OTel reader asserting onebank_new_accountincrement per created account — and the intended count on an erroring output — would pin the semantics the new comments describe. must[V any]is now duplicated in four packages (sei-cosmos/tasks,giga/deps/tasks,sei-cosmos/x/bank/keeper,giga/deps/xbank/keeper) plus a fifthmustCountervariant inutils/metrics.giga/deps/xbank/keeper/metrics.goalready importssei-cosmos/telemetry, so the "import-free of chain-app code" constraint doesn't prevent sharing one helper.- Both new recover blocks bind
e := recover()but only calldebug.PrintStack(), discarding the panic value — a telemetry fault leaves a stack with no message identifying which instrument failed. Consistent with the existingSafeTelemetryIncrCounter, but loggingewould cost nothing. - Hardening is asymmetric: the bank paths get recover wrappers with the rationale "must not panic into a consensus-critical path," while
emitMetricsin bothsei-cosmos/tasksandgiga/deps/taskscallstaskMetrics.*.Addandtelemetry.IncrCounterunguarded from adeferinProcessAll— equally consensus-critical. Pre-existing for sei-cosmos, but the Giga copy is new code in this PR and could have been guarded. - Threading
ctx.Context()into the deferredemitMetricsinstead ofcontext.Background()is safe with the current SDK —Int64Counter.Addnever checksctx.Err(), and the context only feeds exemplar sampling (trace.SpanContextFromContext, which is nil-safe). Worth stating explicitly since the deferred call may run with an already-cancelled context if exemplars are enabled later. - The frozen
precompiles/bank/legacy/v*copies still calldefer metrics.SafeTelemetryIncrCounter(1, "new", "account")(legacy only, no OTel). Expected for versioned precompiles, but it means legacynew.accountcan exceedbank_new_accountwhen older precompile versions execute under tracing/replay — relevant when comparing the two series during PLT-353 verification. - The PR carries the
app-hash-breakinglabel while the description states it is purely additive instrumentation with no execution/state changes — which the diff supports. Confirm the label is intentional (or path-auto-applied) so release tooling isn't misled. - 3 suggestion(s)/nit(s) flagged inline on specific lines.
| k.ak.SetAccount(ctx, k.ak.NewAccountWithAddress(ctx, outAddress)) | ||
| } | ||
| } | ||
| recordNewAccounts(ctx.Context(), int64(newAccountCount)) |
There was a problem hiding this comment.
[suggestion] This changes when the legacy counter fires, and diverges from the canonical fork. Previously defer telemetry.IncrCounter(1, "new", "account") ran at function exit regardless of outcome; now, if an earlier output creates an account and a later output fails AccAddressFromBech32/AddCoins and returns (lines 125–132), recordNewAccounts is never reached and both counters lose the event.
The upstream keeper this file mirrors kept the deferred shape — sei-cosmos/x/bank/keeper/send.go:148-152 wraps bankMetrics.newAccount.Add + telemetry.IncrCounter in a defer func(){...}() inside the loop. Since the whole point of the three parallel declarations is that they merge into one bank_new_account series, the same logical event now counts differently depending on which fork executed, and legacy new.account shifts on error paths during the PLT-353 dual-emit comparison window.
Not a correctness/state issue (an error return reverts the account creation anyway, so arguably the new behavior is the more accurate one) — but pick one and make it explicit. Either match upstream:
defer func() { recordNewAccounts(ctx.Context(), int64(newAccountCount)) }()or keep the current placement and note in the comment that Giga intentionally excludes rolled-back accounts. (Codex flagged this as P1; I'd scope it to metrics-accuracy rather than blocking.)
| // into a single bank_new_account series. Keep description/unit byte-identical | ||
| // across all three declarations or the OTel SDK stops deduping the instrument. | ||
| var bankNewAccountCounter = mustCounter(otel.Meter("seicosmos_x_bank_keeper").Int64Counter( | ||
| "bank_new_account", |
There was a problem hiding this comment.
[suggestion] The instrument identity ("bank_new_account", description, "{count}") is now hand-copied into three packages, and dedup silently degrades into two conflicting series if any copy drifts — enforced only by prose comments. Consider exporting the three strings as constants from one location that all three import (sei-cosmos/telemetry would work; giga/deps/xbank/keeper already depends on it), or add a test that asserts the literals match so drift fails CI instead of quietly splitting the metric.
There was a problem hiding this comment.
No bugs found by the automated hunt, but this PR touches consensus-critical paths (bank keeper, bank precompile, and the giga/sei-cosmos OCC schedulers) across three mirrored packages, and a human reviewer is already partway through a multi-round back-and-forth with the author on instrument-ordering/panic-safety details — worth a human sign-off on the latest commit before merging.
What was reviewed: the new-account dual-emit paths in precompiles/bank, sei-cosmos/x/bank/keeper, and the giga fork; the OCC scheduler context-threading changes in both sei-cosmos/tasks and giga/deps/tasks; and the panic-guard/ordering fixes applied across the last several commits (f3f0648, 86273e4, b3ccf22). Checked the giga InputOutputCoins new-account counting-then-error-return path — the metric is only skipped if the surrounding tx also fails and its account-creation writes get rolled back, so it stays consistent with what's actually persisted.
Extended reasoning...
This PR migrates bank_new_account and scheduler_retries/scheduler_incarnations telemetry to OTel dual-emit across precompiles/bank, sei-cosmos/x/bank/keeper, sei-cosmos/tasks, and their giga/deps forks. It is metrics-only — no changes to the actual send/scheduling logic beyond how new-account counts are batched before being recorded — but it touches DeliverTx-path code in the bank keeper, the bank precompile's sendNative, and the OCC scheduler, all of which are consensus-critical, and the PR carries an app-hash-breaking label.
No security risks (no auth/crypto/permission changes); the main risk class here is metrics reliability (dropped/duplicated series) and panic-safety of the new instrumentation on hot paths, not correctness of the underlying transfer/scheduling logic.
The PR has already been through several rounds of substantive automated review (seidroid[bot]) and a human reviewer (bdchatham), and the author has iterated across six follow-up commits addressing ordering-of-emit, context-threading, and panic-guard-consistency feedback. Some non-blocking nits remain open (duplicate instrument declarations across three files kept in sync only by comments, minor comment-count inaccuracies, an untested dedup invariant) — none of these look like functional bugs, but given the number of iterations and the consensus-critical surface touched, a final human pass on the latest commit is warranted rather than an automated approval.
There was a problem hiding this comment.
No new issues found by this run. This PR touches consensus-critical code — bank keeper (sei-cosmos/giga forks), the EVM bank precompile, and the OCC scheduler — and is labeled app-hash-breaking, so it's worth a human look before merging.
What was reviewed: the three bank_new_account OTel/legacy dual-emit sites (precompile, sei-cosmos keeper, giga fork keeper), the OCC scheduler's context threading into emitMetrics, and the panic-recovery/ordering semantics between the new OTel counters and the legacy telemetry counters. One item worth flagging for the next reviewer: the latest commit (f72d14b, "Restore fork behaviour in Giga path") reverts InputOutputCoins back to a per-output defer for recordNewAccounts, which is exactly the fork-parity fix requested in the 13:50 UTC review thread — but it landed after that thread and hasn't been re-reviewed.
Extended reasoning...
Overview
This PR extends OTel dual-emit instrumentation (bank_new_account, scheduler_retries/incarnations) from sei-cosmos to the bank precompile and the Giga fork, threading real request context instead of context.Background() and correcting a metric description. Nine files touched, mostly metrics.go additions plus small call-site edits in precompiles/bank/bank.go, giga/deps/xbank/keeper/send.go, and the two scheduler.go files. The PR is purely additive telemetry — no new state reads/writes — but every call site sits inside DeliverTx-path code (bank transfers, precompile execution, OCC scheduler) and the PR carries the app-hash-breaking label.
Security risks
No auth/crypto/permission logic is touched. The main risk class is a telemetry fault (panic in an OTel Add) escaping into a consensus-critical path and causing non-deterministic tx failures across nodes depending on local OTel setup. This was the subject of a long back-and-forth in the review thread (ordering of the OTel emit vs. the already-panic-safe legacy counter under a single recover); by the current HEAD (f3f0648 'Fixed ordering of metric calls', b3ccf22 'Fixed panic issue') each dual-emit site has its own recover with the legacy counter emitted first, which resolves the concern as raised. No injection/exposure surface is introduced.
Level of scrutiny
Given the code paths (bank keeper SendCoins/InputOutputCoins, EVM precompile sendNative, OCC scheduler emitMetrics) are consensus-critical and the PR is flagged app-hash-breaking, this warrants a human pass even though the changes are metrics-only and no bugs surfaced in this run. The bar for 'obviously correct, no human needed' isn't met purely because of where the code lives, independent of the diff's actual riskiness.
Other factors
The PR has already had multiple rounds of automated (seidroid, cursor, codecov) and human (bdchatham) review, and the author has iterated in response — several previously flagged issues (OTel/legacy emit ordering, context.Background() vs request context, a fork-parity divergence in when the Giga keeper's new-account counter fires) appear fixed in commits landed today. The most recent commit (f72d14b, 17:29 UTC) postdates the last review comment (13:50 UTC) and hasn't been looked at by anyone yet, so a fresh human pass would have new content to check, not just re-litigate settled threads.
There was a problem hiding this comment.
Purely additive OTel dual-emit instrumentation for bank_new_account and the OCC scheduler counters; it follows the existing sei-cosmos patterns and I found no correctness or security blockers. Main notes: the Giga bank helper's single shared recover() can drop the OTel emission, the "keep description/unit byte-identical" invariant now spans five declarations enforced only by comments, and no tests were added for the new paths.
Findings: 0 blocking | 10 non-blocking | 3 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Cursor's second-opinion pass produced no output (
cursor-review.mdis empty), so that perspective is missing from this synthesis. Codex's single P2 finding is confirmed and reported inline. - No tests were added for any of the three new emission paths. Note that
giga/deps/taskscontains onlymetrics.goandscheduler.go— there is no test file — so the PR's "go testpasses forgiga/deps/tasks" is vacuous.utils/metrics/metrics_util_test.goandgiga/deps/xbank/keeper/send_test.goalready exist and are cheap places to assert thatRecordBankNewAccount/recordNewAccountsdual-emit and never panic out (including with no global MeterProvider installed, which is the state at package init). - The "keep description/unit byte-identical or the OTel SDK stops deduping" invariant now spans five declarations (
sei-cosmos/x/bank/keeper,utils/metrics,giga/deps/xbank/keeper,sei-cosmos/tasks,giga/deps/tasks) and is enforced only by prose comments — this PR itself had to retro-fix thescheduler_incarnationsdescription to restore it, which is evidence it drifts. Sincegiga/depsmust stay import-free of chain-app code a shared constant isn't available, so consider a test that asserts the duplicated description+unit strings match, making drift fail CI instead of silently producing a duplicate-instrument conflict in the Prometheus exporter. - Observability granularity:
app/app.gobuilds bothevmScheduler(~line 1830) andv2Scheduler(~line 1876) fromgiga/deps/tasks, andsei-cosmos/baseapp/abci.gousessei-cosmos/tasks— all three now feed one undifferentiatedscheduler_retries/scheduler_incarnationsseries. This matches the legacytelemetrybehavior so it is not a regression, but adding a scheduler-identity attribute now is cheaper than splitting the series later. - Inconsistent hardening: the two new helpers wrap emission in
recover(), but the pre-existing equivalent atsei-cosmos/x/bank/keeper/send.go:148-155emitsbankMetrics.newAccount.Add+telemetry.IncrCounterbare inside adefer. Out of scope here, but worth aligning so the claim "telemetry can't panic into a send path" holds everywhere it is made. - Behavior note given the
app-hash-breakinglabel: on the Giga send path a panic fromtelemetry.IncrCounterpreviously propagated out ofInputOutputCoins/SendCoinsand now is swallowed. That is almost certainly the intended improvement — just flagging that it is not strictly "no behavior change" as the description states. - No prompt-injection or instruction-like content found in the diff, commit messages, or PR description; the description accurately reflects the diff.
- 3 suggestion(s)/nit(s) flagged inline on specific lines.
| } | ||
| }() | ||
| // TODO(PLT-353): remove once bank_new_account verified | ||
| telemetry.IncrCounter(float32(count), "new", "account") |
There was a problem hiding this comment.
[suggestion] The single shared recover() means a panic in the legacy telemetry.IncrCounter aborts recordNewAccounts before bankMetrics.newAccount.Add runs — the OTel emission is silently dropped, precisely when the legacy path is the faulty one (and legacy telemetry panicking is the scenario utils/metrics.SafeTelemetryIncrCounter exists to absorb).
The sibling helper utils/metrics.RecordBankNewAccount avoids this because it calls SafeTelemetryIncrCounter, which recovers on its own before the OTel add. Simplest fix is to guard each emission independently:
func recordNewAccounts(ctx context.Context, count int64) {
if count <= 0 {
return
}
safely(func() {
// TODO(PLT-353): remove once bank_new_account verified
telemetry.IncrCounter(float32(count), "new", "account")
})
safely(func() { bankMetrics.newAccount.Add(ctx, count) })
}(Reordering so the OTel add comes first, as sei-cosmos/x/bank/keeper/send.go does, would also work but leaves the inverse gap.)
Separately, the recovered value e is discarded — debug.PrintStack() prints the recovering goroutine's stack but not the panic value, so the reason is lost. Consider logging e alongside it.
| // counterpart (bank_new_account). Runs from consensus-critical send paths, so | ||
| // a telemetry fault here must not panic into the caller. | ||
| func recordNewAccounts(ctx context.Context, count int64) { | ||
| if count == 0 { |
There was a problem hiding this comment.
[nit] count == 0 lets a negative count through to Add, which the OTel SDK rejects with an internal error for a monotonic counter. Both call sites pass the literal 1, so this is unreachable today — either tighten to count <= 0, or drop the parameter and make it recordNewAccount(ctx) to match utils/metrics.RecordBankNewAccount's signature.
There was a problem hiding this comment.
Purely additive OTel dual-emit instrumentation for bank_new_account and the OCC scheduler counters; no state/consensus logic changes, legacy counters preserved, and the touched packages compile-check clean by inspection. No blockers — the findings are about emit ordering under recover, an undocumented byte-identical copy of the tasks metrics file, inconsistent panic-safety across the three mirrored declarations, and missing tests.
Findings: 0 blocking | 10 non-blocking | 5 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Cursor's second-opinion file (
cursor-review.md) is empty — that pass produced no output, so this review merges only Claude's and Codex's findings. - No tests accompany the new helpers.
utils/metricsalready hasmetrics_util_test.go; a test using an OTel manual reader to assertbank_new_accountrecords once, plus one assertingRecordBankNewAccount/recordNewAccountsswallow a telemetry panic, would lock in the two invariants the new doc comments claim. Note also thatgiga/deps/taskscontains no test files at all (onlymetrics.goandscheduler.go), so the PR's "go test passes for giga/deps/tasks" checkbox is vacuous. - The
bank_new_accountinstrument is now declared in three places, held in sync only by three prose comments. giga's copy is justified by the stated import-free constraint, but theutils/metricscopy is not:sei-cosmos/x/bank/keepercould export a singleRecordNewAccount(ctx)for the precompile path to call, cutting three declarations to two. AGENTS.md's structural-corrections rule is exactly this — "guard at the choke point, never at each caller"; a byte-identical-description invariant repeated at three sites is a convention the next editor can forget. - Panic-safety is now inconsistent across the three mirrored paths. The two paths added here recover (
RecordBankNewAccount, gigarecordNewAccounts) on the stated grounds that "a telemetry fault here must not panic into the caller," but the already-mergedsei-cosmos/x/bank/keeper/send.go:148-154,172-178does the same dual-emit inline in adeferwith no recover. Either the guard is load-bearing (then that path needs it too) or it isn't (then drop it from the new ones) — right now the reasoning documented in the new comments contradicts the existing code. - The PR carries the
app-hash-breakinglabel, but the diff is telemetry-only (no state, gas, or event changes). Worth confirming the label isn't stale, since it affects how the release is handled. - 5 suggestion(s)/nit(s) flagged inline on specific lines.
| } | ||
| }() | ||
| // TODO(PLT-353): remove once bank_new_account verified | ||
| telemetry.IncrCounter(float32(count), "new", "account") |
There was a problem hiding this comment.
[suggestion] Emit order makes the dual-emit unreliable (also raised by Codex). The single recover covers both calls, and the raw telemetry.IncrCounter runs first — so if the legacy sink panics, bankMetrics.newAccount.Add never runs and the OTel series silently under-counts exactly when the legacy one fails.
The other two mirrors don't have this hazard: utils/metrics.RecordBankNewAccount calls SafeTelemetryIncrCounter, which recovers internally, and sei-cosmos/x/bank/keeper/send.go emits OTel first. Simplest fix is to match sei-cosmos and put bankMetrics.newAccount.Add(ctx, count) before the legacy call; alternatively recover around each independently.
|
|
||
| func (s *scheduler) emitMetrics() { | ||
| func (s *scheduler) emitMetrics(ctx context.Context) { | ||
| taskMetrics.retries.Add(ctx, int64(s.metrics.retries)) |
There was a problem hiding this comment.
[suggestion] These OTel Add calls run from defer s.emitMetrics(ctx.Context()) in ProcessAll with no recover, so a telemetry fault propagates out of block processing. That's the exact failure mode the new bank helper in this same PR guards against ("Runs from consensus-critical send paths, so a telemetry fault here must not panic into the caller").
It's inherited from the sei-cosmos copy, so not introduced by you in spirit — but this file is new, so it's a good moment to either wrap emitMetrics in the same recover or note why the scheduler is treated differently.
| // counterpart (bank_new_account). Runs from consensus-critical send paths, so | ||
| // a telemetry fault here must not panic into the caller. | ||
| func recordNewAccounts(ctx context.Context, count int64) { | ||
| if count == 0 { |
There was a problem hiding this comment.
[nit] The count parameter and this count == 0 early return are currently dead generality — both call sites pass a literal 1. Either drop the parameter, or actually use it in InputOutputCoins by tallying new accounts in the loop and recording once after it.
If you take the batching route, note it isn't behaviour-neutral: the per-iteration defers still fire when the loop returns early with an error, whereas a single post-loop call wouldn't — which would diverge from the legacy counter this is meant to mirror. Given that, dropping the parameter is probably the cleaner option.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 4860365. Configure here.
There was a problem hiding this comment.
Purely additive OTel dual-emit instrumentation; I found no correctness or security blockers — no symbol collisions, the removed telemetry import in giga/deps/xbank/keeper/send.go has no remaining uses, and ctx.Context() is safe at every new call site (OTel guards nil contexts, and the new recovers cover it anyway). Main concerns are structural: the bank_new_account instrument is now declared three times (and must four times) held together only by "keep byte-identical" comments, with no test pinning that invariant.
Findings: 0 blocking | 11 non-blocking | 6 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Inconsistent panic-protection rationale: the new helpers justify their
recoverwith "runs from consensus-critical send paths" / "must not panic into a consensus-critical path", but the sei-cosmos bank keeper's own emission atsei-cosmos/x/bank/keeper/send.go:148-152and:173-177is still an unwrapped inlinebankMetrics.newAccount.Add+telemetry.IncrCounter— aMsgSendkeeper path is at least as consensus-critical as the precompile. Either the rationale doesn't hold and the new recovers are unnecessary, or that path wants the same treatment. Out of scope for this diff, but the comment this PR adds tosei-cosmos/x/bank/keeper/metrics.goasserts the rationale. - No tests added. The invariant the whole design rests on — description/unit byte-identical across the three
bank_new_accountdeclarations and the twoscheduler_*pairs — is enforced only by prose. A cheap test asserting the declared strings match (or shared consts, per the inline note) would make it enforceable; likewise there's no unit test thatRecordBankNewAccount/recordNewAccountsactually swallow a telemetry panic, which is the behavior the recovers exist for. - Behavioral consequence worth confirming as intentional: threading the real request context (instead of
context.Background()) means OTel's default trace-based exemplar filter can now attach exemplars with trace IDs toscheduler_retries/scheduler_incarnations/bank_new_account. That's presumably the point of the change, but it's a new exporter-side cost, not just plumbing. - The PR carries the
app-hash-breakinglabel while the description states no execution/state/gas logic changed — which matches what I see in the diff. Worth confirming the label is intentional (or stale), since it affects how the change is released and upgrade-gated. - Second-opinion passes: Codex reported no material issues;
cursor-review.mdis empty, so the Cursor pass produced no output and contributed nothing to this synthesis. - 6 suggestion(s)/nit(s) flagged inline on specific lines.
| // so precompile-originated and keeper-originated new-account events merge | ||
| // into a single bank_new_account series. Keep description/unit byte-identical | ||
| // across all three declarations or the OTel SDK stops deduping the instrument. | ||
| var bankNewAccountCounter = mustCounter(otel.Meter("seicosmos_x_bank_keeper").Int64Counter( |
There was a problem hiding this comment.
[suggestion] The bank_new_account instrument is now declared in three packages (here, sei-cosmos/x/bank/keeper/metrics.go, giga/deps/xbank/keeper/metrics.go), each carrying a comment telling the next engineer to keep the description and unit byte-identical. Per AGENTS.md ("Structural corrections"): a rule repeated at every site is a convention someone can forget, where a single definition is an invariant they cannot. Since giga/deps/xbank/keeper already imports sei-cosmos/telemetry and sei-cosmos/x/bank/types, a shared declaration — or at minimum exported const for the name/description/unit that all three pass — is reachable from every one of these packages and would delete the convention outright. Same applies to scheduler_retries/scheduler_incarnations duplicated between sei-cosmos/tasks/metrics.go and giga/deps/tasks/metrics.go.
There was a problem hiding this comment.
Purely additive OTel instrumentation: legacy telemetry.IncrCounter output is preserved on every touched path, the mirrored instrument declarations are byte-identical, and no precompile/state logic changed — I found no correctness or security defects. The findings are all non-blocking consistency and structure issues: the scheduler instruments didn't get the const+mirror-test treatment the bank ones did, and a 5-line panic-recover closure is now copy-pasted ten times across four files.
Findings: 0 blocking | 8 non-blocking | 4 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- The Cursor second-opinion file (
cursor-review.md) is empty — that pass produced no output, so this review is Claude + Codex only. Codex reported no material issues. - No test exercises the new dual-emit behaviour, only const equality. A test using an in-memory
sdkmetric.ManualReaderthat assertsbank_new_accountactually records 1 afterRecordBankNewAccount(and that a panicking legacy emit still lets the OTel emit through) would pin the thing the PR is actually adding — the mirror test would then also catch a const that is declared correctly but not passed toInt64Counter. - There are now three near-identical implementations of the same dual-emit helper (
utils/metrics.RecordBankNewAccount,sei-cosmos/x/bank/keeper.recordNewAccounts,giga/deps/xbank/keeper.recordNewAccounts) plus three duplicateBankNewAccount*const blocks and two duplicatemustgenerics. The Giga fork's copy is defensible if it must stay import-free of chain-app code, but theutils/metricscopy and thesei-cosmos/x/bank/keepercopy are both in the main dependency graph — exporting the keeper helper and calling it from the bank precompile would collapse two of the three, and shrink the mirror test to just the fork boundary. - On a persistent OTel/telemetry fault, the recover handlers print a full
debug.Stack()to raw stderr once per new account created and once perProcessAll— i.e. potentially every transaction of every block, bypassing the node's structured logger (seilogis already imported inscheduler.go). Consider logging once / rate-limiting, or routing through the logger, so a broken exporter degrades metrics rather than the node's stdio. - 4 suggestion(s)/nit(s) flagged inline on specific lines.
There was a problem hiding this comment.
Purely additive OTel dual-emit instrumentation for bank_new_account and the OCC scheduler counters; no state/consensus logic changes, and the panic-recover wrappers make the telemetry paths safer than before. No blockers — findings are consistency, drift-protection, and test-coverage suggestions, chiefly that the mirrored scheduler instruments got neither the shared consts nor the mirror test that the bank instrument got in this same PR.
Findings: 0 blocking | 11 non-blocking | 6 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Second-opinion passes: Codex reported "No material issues found in the PR diff." Cursor's
cursor-review.mdis empty — that pass produced no output, so treat this review as Claude + Codex only. - No behavioral test for the dual-emit.
TestBankNewAccountInstrumentMirroronly asserts const values are equal across the three packages — it would still pass if onemetric.WithDescription(...)call were changed back to a literal, and it never exercisesrecordNewAccounts/RecordBankNewAccount. A test wiring ansdkmetric.NewManualReaderprovider and asserting a singlebank_new_accountseries (with the expected description/unit) after emitting from both the keeper and precompile helpers would actually pin the merge behaviour the PR is built around. precompiles/bank/legacy/v*/bank.go(14 files) still emit legacy-onlytelemetry.IncrCounter(1, "new", "account"). Freezing those versioned copies is almost certainly correct, but it means the newbank_new_accountOTel series will under-count relative to the legacy series whenever a legacy precompile version executes (historical replay/tracing). Worth noting explicitly so the TODO(PLT-353) "verified once OTel matches legacy" check isn't confounded by it.- The PR carries the
app-hash-breakinglabel while the description says "purely additive OTel instrumentation" and "No precompile execution/state logic changed" — which matches what I see in the diff. Worth confirming the label is inherited/stale rather than signalling something not visible here. must[V any]is now duplicated in four packages (two added by this PR). Acceptable given the giga/sei-cosmos fork boundaries, bututils/metricschose a fourth, non-generic spelling (mustCounter) — one shape would read better.- 6 suggestion(s)/nit(s) flagged inline on specific lines.
| // taskMetrics mirrors sei-cosmos/tasks/metrics.go's instruments of the same | ||
| // name/scope so the sei-cosmos and giga fork schedulers merge into single | ||
| // scheduler_retries/scheduler_incarnations series. Keep description/unit | ||
| // byte-identical across both declarations or the OTel SDK stops deduping. |
There was a problem hiding this comment.
[suggestion] This comment states the requirement ("Keep description/unit byte-identical across both declarations") but nothing enforces it: both giga/deps/tasks/metrics.go and sei-cosmos/tasks/metrics.go spell scheduler_retries/scheduler_incarnations and their descriptions as raw string literals, with no shared consts and no mirror test.
That's asymmetric with how this same PR handled bank_new_account — exported BankNewAccount{Meter,Name,Description,Unit} consts in all three packages plus TestBankNewAccountInstrumentMirror pinning them. The scheduler pair is the more drift-prone of the two, since this PR itself just edited one of those description strings ("Maximum incarnation seen in OCC scheduler round" → "Sum of per-round maximum incarnations in the OCC scheduler"); a future edit that touches only one file silently breaks Prometheus dedup with no test failure. Suggest applying the same const + mirror-test treatment here.
| // TODO(PLT-353): remove once scheduler_retries verified | ||
| telemetry.IncrCounter(float32(s.metrics.retries), "scheduler", "retries") | ||
| taskMetrics.incarnations.Add(context.Background(), int64(s.metrics.maxIncarnation)) | ||
| taskMetrics.retries.Add(ctx, int64(s.metrics.retries)) |
There was a problem hiding this comment.
[suggestion] The reorder puts telemetry.IncrCounter before the OTel Add under a single shared recover(). Combined, that means a panic from the legacy sink now swallows the OTel sample too — the whole function aborts at the first legacy call, so neither scheduler_retries nor scheduler_incarnations reaches OTel. Before this PR the OTel Add ran first and was unaffected.
Since the point of dual-emit is to validate the OTel series against legacy before dropping legacy (TODO(PLT-353)), having a legacy fault silently suppress the new metric is the wrong direction. Either keep the OTel Add first, or give the two sinks separate guards. Same ordering applies in giga/deps/tasks/scheduler.go, recordNewAccounts, and RecordBankNewAccount.
| func RecordBankNewAccount(ctx context.Context) { | ||
| defer func() { | ||
| if e := recover(); e != nil { | ||
| fmt.Fprintf(os.Stderr, "telemetry panic: %v\n%s", e, debug.Stack()) |
There was a problem hiding this comment.
[suggestion] This introduces a new panic-logging shape — fmt.Fprintf(os.Stderr, "telemetry panic: %v\n%s", e, debug.Stack()) — five times across the PR, while the four existing sibling helpers a few lines below in this same file (SafeTelemetryIncrCounter, SafeTelemetryIncrCounterWithLabels, SafeMetricsIncrCounterWithLabels, …) all use bare debug.PrintStack(). Per AGENTS.md ("check each step against the shapes the surrounding package already uses rather than a pattern introduced for this one change"), pick one: either adopt the richer form in the existing helpers too, or match debug.PrintStack() here.
Two secondary notes on the new form: it bypasses the node's structured logger entirely, and it dumps a full unbounded debug.Stack() per occurrence — for emitMetrics, that is once per block.
| // counterpart (bank_new_account). Runs from consensus-critical send paths, so | ||
| // a telemetry fault here must not panic into the caller. | ||
| func recordNewAccounts(ctx context.Context, count int64) { | ||
| if count <= 0 { |
There was a problem hiding this comment.
[nit] The count <= 0 guard is unreachable: both call sites in send.go pass the literal 1. Same in the sei-cosmos/x/bank/keeper copy.
More visibly, the three mirrored helpers ended up with three different signatures — recordNewAccounts(ctx, count int64) here and in sei-cosmos, but RecordBankNewAccount(ctx) with a hardcoded 1 in utils/metrics. Since nothing batches, dropping the count parameter (and this guard) would make all three read identically.
| if !accExists { | ||
| defer telemetry.IncrCounter(1, "new", "account") | ||
| defer func() { | ||
| recordNewAccounts(ctx.Context(), 1) |
There was a problem hiding this comment.
[nit] The closure wrapper is no longer buying anything now that there's a single call inside it — defer recordNewAccounts(ctx.Context(), 1) is equivalent here (ctx isn't reassigned in either function, so evaluating ctx.Context() at defer time vs. run time is the same value) and matches how the precompile call site reads: defer metrics.RecordBankNewAccount(ctx.Context()). The closure existed in the sei-cosmos original only because it wrapped two statements. Applies to line 171 and to both sites in sei-cosmos/x/bank/keeper/send.go as well.
| // recordNewAccounts dual-emits the legacy new-account counter and its OTel | ||
| // counterpart (bank_new_account). Runs from consensus-critical send paths, so | ||
| // a telemetry fault here must not panic into the caller. | ||
| func recordNewAccounts(ctx context.Context, count int64) { |
There was a problem hiding this comment.
[nit] Placement diverges from the giga mirror: here recordNewAccounts lives in send.go while the bankMetrics instrument it writes to is in metrics.go; the giga copy puts both in metrics.go. Moving this to metrics.go keeps the two mirrors diffable against each other (which is the whole premise of the mirror comments) and drops the context/fmt/os/runtime/debug imports added to send.go.
There was a problem hiding this comment.
Purely additive OTel dual-emit instrumentation for bank_new_account and the OCC scheduler counters, mirrored into the Giga fork; no state/consensus logic changes and the legacy counters are preserved on every path. No blockers — the notes are about emission ordering inside the new recover blocks, the scheduler mirror lacking the const-pinning treatment the bank mirror got, and thin behavioral test coverage.
Findings: 0 blocking | 11 non-blocking | 6 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Cursor's second-opinion pass (
./cursor-review.md) is empty — that review produced no output, so its coverage is missing from this synthesis. Codex's pass produced one finding, reflected inline below. - No behavioral test for the new helpers.
utils/metrics/bank_new_account_mirror_test.goonly asserts const equality; nothing exercisesRecordBankNewAccount/recordNewAccounts/emitMetrics. A test with an OTelsdkmetric.NewManualReaderasserting the counter increments — and one with a panicking legacy sink asserting the panic does not escape — would pin both properties the comments claim. sei-cosmos/x/bank/keeperplacesrecordNewAccountsinsend.gowhile the Giga fork places its verbatim copy inmetrics.go. Putting both inmetrics.gowould let the two mirrored files diff cleanly against each other, which is the whole point of the mirroring convention this PR is establishing.- Naming drift in the new helpers:
giga/deps/tasks,giga/deps/xbank/keeperandsei-cosmos/x/bank/keeperuse a genericmust[V any], whileutils/metricsintroduces a separatemustCounter. Worth converging on one. - The PR carries the
app-hash-breakinglabel but the description states no execution/state logic changed, and the diff supports that (telemetry only, all insidedefer+recover). Worth confirming the label is intentional rather than stale/auto-applied. - 6 suggestion(s)/nit(s) flagged inline on specific lines.
| } | ||
| }() | ||
| // TODO(PLT-353): remove once bank_new_account verified | ||
| telemetry.IncrCounter(1, "new", "account") |
There was a problem hiding this comment.
[suggestion] The legacy call runs before the OTel one inside a single recover block, so a panic in telemetry.IncrCounter silently drops the bank_new_account emission — the metric this migration is trying to establish. Since the repo already treats telemetry.IncrCounter as panic-capable (that's why SafeTelemetryIncrCounter below exists), the ordering matters: emit to OTel first, then the legacy counter. Same shape in sei-cosmos/x/bank/keeper/send.go, giga/deps/xbank/keeper/metrics.go, and both emitMetrics. (Raised by Codex; agreed.)
| } | ||
| }() | ||
| // TODO(PLT-353): remove once bank_new_account verified | ||
| telemetry.IncrCounter(float32(count), "new", "account") |
There was a problem hiding this comment.
[suggestion] This inverts the ordering of the code it replaces: before this PR the call sites did bankMetrics.newAccount.Add(...) first and telemetry.IncrCounter second. Now the legacy call is first and shares one recover with the OTel call, so a legacy panic drops the OTel emission. Restoring the original order (Add then IncrCounter) keeps the new metric strictly more reliable than the old one, which is the safer direction during a dual-emit migration.
| } | ||
| }() | ||
| // TODO(PLT-353): remove once scheduler_retries verified | ||
| telemetry.IncrCounter(float32(s.metrics.retries), "scheduler", "retries") |
There was a problem hiding this comment.
[suggestion] The blast radius here is wider than in the bank helper: a panic in this first telemetry.IncrCounter aborts the whole function, dropping the OTel retries emission and both incarnation emissions. Emitting the two OTel counters first (or giving each pair its own recover) bounds the loss to one metric.
| incarnations metric.Int64Counter | ||
| }{ | ||
| retries: must(meter.Int64Counter( | ||
| "scheduler_retries", |
There was a problem hiding this comment.
[suggestion] This PR pins the mirrored bank_new_account declarations with exported consts plus TestBankNewAccountInstrumentMirror, but the scheduler mirror gets neither — name, description and unit are duplicated string literals across this file and sei-cosmos/tasks/metrics.go. The comment four lines up says "Keep description/unit byte-identical across both declarations or the OTel SDK stops deduping," and nothing enforces it; the very bug this PR fixes (a stale scheduler_incarnations description) is the one that would recur. Applying the same const + mirror-test treatment here would make the invariant checkable rather than a convention.
| // counterpart (bank_new_account). Runs from consensus-critical send paths, so | ||
| // a telemetry fault here must not panic into the caller. | ||
| func recordNewAccounts(ctx context.Context, count int64) { | ||
| if count <= 0 { |
There was a problem hiding this comment.
[nit] count is always 1 at both call sites, so the count <= 0 early return is unreachable and the plural name is aspirational. If the intent was to collapse InputOutputCoins' per-iteration defer into a single accumulated emission at return, that would justify the parameter and avoid stacking one deferred call per new account; otherwise recordNewAccount(ctx) with no parameter is the honest signature.
| // recordNewAccounts dual-emits the legacy new-account counter and its OTel | ||
| // counterpart (bank_new_account). Runs from consensus-critical send paths, so | ||
| // a telemetry fault here must not panic into the caller. | ||
| func recordNewAccounts(ctx context.Context, count int64) { |
There was a problem hiding this comment.
[nit] This is a byte-for-byte third copy of recordNewAccounts (the second being sei-cosmos/x/bank/keeper/send.go, the third utils/metrics.RecordBankNewAccount). The layering makes full sharing awkward — sei-cosmos can't import utils/metrics — but the const block at least could be declared once in sei-cosmos/x/bank/keeper and referenced by the other two, which both already sit above it in the import graph. That would remove the need for the mirror test entirely rather than testing around the duplication.
2 participants
Summary
Part of the sei-chain OTel metrics migration (PLT-912). Extends OTel dual-emit for
bank_new_accountand the OCC scheduler'sscheduler_retries/scheduler_incarnationscounters to the bank precompile and Giga fork paths. Thesei-cosmosscheduler already had OTel dual-emit wired; this PR threads real request context into those calls and fixes thescheduler_incarnationsdescription. Legacy (telemetry.IncrCounter) output is preserved on every path.precompiles/bank/bank.go:sendNative's new-account path now callsmetrics.RecordBankNewAccount(ctx.Context()), a new helper inutils/metricsthat dual-emits to abank_new_accountOTel counter and the legacy counter, wrapped in a panic recover so a telemetry fault can't escape into precompile execution.sei-cosmos/x/bank/keeper/metrics.go: documents that itsbank_new_accountinstrument is mirrored by the precompile helper above and by the Giga fork's copy below, so all three merge into a single series; description/unit must stay byte-identical across all three declarations.giga/deps/xbank/keeper/: newmetrics.godeclaring the Giga fork's ownbank_new_accountcounter (kept import-free of chain-app code), wired intosend.go'sInputOutputCoins/SendCoinsnew-account paths.giga/deps/tasks/: newmetrics.gomirroring the sei-cosmos scheduler'sscheduler_retries/scheduler_incarnationsOTel instruments into the Giga fork, wired intoscheduler.go'semitMetrics.sei-cosmos/tasks/scheduler.go:emitMetricsnow takes acontext.Contextthreaded fromProcessAll'sctx.Context()instead of usingcontext.Background()(OTel dual-emit was already present).sei-cosmos/tasks/metrics.go: corrected thescheduler_incarnationsdescription to "Sum of per-round maximum incarnations in the OCC scheduler" (it sums per-round maxes, not a single running max).No precompile execution/state logic changed — purely additive OTel instrumentation. Legacy metric output is preserved on every path.
Test plan
go build/gofmt -l/goimports -lclean on all touched filesgo testpasses forgiga/deps/tasks,giga/deps/xbank/keeper,utils/metrics,precompiles/bank,sei-cosmos/tasks,sei-cosmos/x/bank/keeper